feat(amd): move AMD multi-node fixed-sequence benchmarks to srt-slurm - #2542
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
cquil11
changed the title
3d6a75a to
0641575
Compare
cquil11
changed the title
|
Claude finished @cquil11's task in 8m 2s —— View job PR Review — feat(amd): orchestrate AMD clusters with srt-slurm
SummaryNo blocking issues found. Two 🟡 warnings posted inline:
Checks that passed: Note: I could not execute |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review ran; one nit-level bug was found and posted inline (MI355X qwen3.5 staging writing to the wrong HF cache root). Beyond that, a few other candidates were checked and ruled out: the producer-SHA file written by the srt-slurm launchers, the MI355X sglang disagg conc-list vs. the custom benchmark's actual concurrency sweep, and the AMD_BRINGUP.md translation/pin-staleness questions — none of these turned out to be functional issues. Given the size and novelty of this orchestration integration (new launcher scripts, cluster profiles, and recipes), a human look is still worthwhile.
Extended reasoning...
This PR introduces a large, brand-new AMD orchestration surface (srt-slurm integration): two new launcher scripts with nontrivial Slurm/Pyxis staging logic, two cluster profiles, five new recipe YAMLs, and two new contract test files, spanning MI300X and MI355X. Given the scale, the novelty of the code paths (remote staging via sbatch/srun, cache-path plumbing, git-pinned runtime fetch), and that CI already flags this PR for CODEOWNER sign-off, this falls well outside the bar for full automated approval regardless of the bug severity found. The one inline finding is a nit (redundant download risk, not a hard failure per the PR's own successful validation run), so it does not itself block merging, but the overall change warrants a human pass.
72ee996 to
2182835
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
eb12f0e to
cfd30df
Compare
|
Closing because the project scope is now limited to porting existing active, nondeprecated AMD multi-node configurations. This validation or generic upstreaming PR is outside that final scope. |
|
Reopening: this PR predates the recent out-of-scope configuration-porting detour and was closed by mistake during an overly broad cleanup. Its prior state is being restored. |
| if [[ ! -d "\$srt_runtime/.git" ]]; then | ||
| git clone --quiet "${SRT_SLURM_REPOSITORY}" "\$srt_runtime" | ||
| fi | ||
| git -C "\$srt_runtime" fetch --quiet origin "${SRT_SLURM_COMMIT}" | ||
| git -C "\$srt_runtime" checkout --quiet --detach "${SRT_SLURM_COMMIT}" | ||
| test "\$(git -C "\$srt_runtime" rev-parse HEAD)" = "${SRT_SLURM_COMMIT}" | ||
| make -C "\$srt_runtime" --no-print-directory setup-compute ARCH=x86_64 |
There was a problem hiding this comment.
🔴 Unlocked concurrent git clone/checkout/make on shared srt-slurm runtime dir keyed only by commit, not per-run
Extended reasoning...
REMOTE_SRT_RUNTIME="${REMOTE_BASE}/runtime/srt-slurm-${SRT_SLURM_COMMIT}" is a node-local path shared by ALL runs pinned to the same commit (not namespaced by RUN_KEY), and this directory later becomes SRTCTL_RUNTIME_SOURCE_DIR for the actual serving containers (line 134). Unlike the two container-image imports two lines above, which are explicitly wrapped in a per-target flock, the git clone/fetch/checkout --detach/make setup-compute sequence at lines 84-90 has no locking at all. If two overlapping CI executions (e.g. the srt-agg and srt-disagg MI300X matrix rows, or a rerun overlapping a still-running job) each submit their own 5-node stage sbatch job and Slurm schedules both onto the same compute node at the same time, both srun tasks race on the same .git directory: one process's git checkout --detach can collide with another's git clone/fetch (e.g. "Unable to create '.git/index.lock': File exists", or a clone racing a concurrent checkout leaving a partially-populated worktree). Because the whole stage step runs under set -euo pipefail, any such git failure aborts the entire…
Verification: normal. In runners/launch_mi300x-amds-srt.sh the per-node staging srun wraps container-image imports in a per-target flock (ensure_container_image: flock -w 2400 "$lock_fd"), but the git clone/fetch/checkout --detach/make setup-compute sequence (the if [[ ! -d "$srt_runtime/.git" ]]; then git clone ... block through make -C "$srt_runtime" ... setup-compute) has no locking.…
| exec {lock_fd}>"\${target}.lock" | ||
| flock -w 2400 "\$lock_fd" | ||
| if ! unsquashfs -s "\$target" >/dev/null 2>&1; then | ||
| tmp="\${target}.tmp.\${SLURM_JOB_ID}" | ||
| rm -f "\$tmp" | ||
| enroot import -o "\$tmp" "docker://\${image}" | ||
| unsquashfs -s "\$tmp" >/dev/null | ||
| mv "\$tmp" "\$target" | ||
| fi | ||
| flock -u "\$lock_fd" | ||
| exec {lock_fd}>&- | ||
| } |
There was a problem hiding this comment.
🟡 sweep:tmp="${target}.tmp.${SLURM_JOB_ID}" -- staged squashfs import writes to a per-job .tmp. path (mi300x-amds-srt.sh:73, mi355x-amds-srt.sh:50) with no trap/cleanup on failure; only success path mvs it away.
Extended reasoning...
If enroot import or the follow-up unsquashfs -s fails or the 45-min staging job times out mid-import (network hiccup, oversized ROCm image, node reboot), the partially-written multi-GB .tmp.<jobid> squashfs is left on node-local /raid (mi300x) or the shared /it-share filesystem (mi355x). The next run always picks a fresh SLURM_JOB_ID for its tmp name, so it never revisits or removes the stale file, and the rm -f "$tmp" guard only ever clears the current job's own (not-yet-created) tmp path. Repeated failed CI attempts on the same lane accumulate orphaned multi-GB files, eventually exhausting node-local RAID or the shared HF-cache filesystem used by every other AMD lane. A correct fix cleans stale *.tmp.* siblings of the target (e.g. via a glob-and-remove pass under the lock, or an EXIT trap) before/at the start of ensure_container_image.
Verification: nit. At runners/launch_mi300x-amds-srt.sh:73-77 (and mi355x-amds-srt.sh ~50) the staging import writes to a per-job path tmp="${target}.tmp.${SLURM_JOB_ID}", and the only cleanup is rm -f "$tmp" on line 74 which removes only the current job's own (not-yet-created) tmp. Under set -euo pipefail, if enroot import -o "$tmp" (line 75) or unsquashfs -s "$tmp" (line 76) fails, or the…
cfd30df to
8581966
Compare
6d439d6 to
fd4d3a3
Compare
a3f971a to
0f87045
Compare
0f87045 to
a63bd37
Compare
cquil11
changed the title
f5d2713 to
e97493d
Compare
Per-allocation host checks live in runners/srt-slurm/hooks/<cluster>/setup.sh and run through the cluster profile's default_host_setup. Shared host-check functions move to hooks/common.sh. MI355X gets its RDMA/QoS preflight, GPU drain gate, and hugepage reclaim.
Multi-node jobs with a CONFIG_FILE use the shared native path: srt-slurm owns allocation, serving, and post-eval; results and eval artifacts are collected with the shared helpers. The MI355X profile adds the multi-node fabric, model, and cache settings.
Move the Qwen3.5 FP8/MXFP4 and DeepSeek-R1 FP8/MXFP4 disaggregated configs to srt-slurm recipes. Recipes write results in the shared sa-bench layout, and eval runs drop fake expert dispatch. The legacy multi-node path now only serves AgentX.
Delete the retired MI355X disaggregated launch scripts, the amd_utils vLLM and ATOM engine paths, the fixed-sequence bench client, and the per-model fixed-sequence server settings. AgentX keeps its SGLang and TileRT paths.
e97493d to
97a721a
Compare
The cached Hub snapshot's config is text-only (qwen3_5_moe_text, no vision_config), so SGLang's Qwen-VL processor crashed at startup. The legacy path loaded /it-share/data/Qwen3.5-397B-A17B-FP8; register that as a model alias and use it in the recipe.
1 participant
Moves AMD multi-node fixed-sequence benchmarks to srt-slurm and removes the legacy bash that coordinated them. Follows #3352 (merged), which moved single-node fixed-sequence.
runners/srt-slurm/hooks/<cluster>/setup.sh, run through the profile'sdefault_host_setup, with shared helpers inhooks/common.sh. MI355X gets its RDMA/QoS preflight, GPU drain gate, and hugepage reclaim. Documented inAGENTS.mdanddocs/configuration-procedures.md.runners/launch_mi355x-amds.sh: multi-node jobs with aCONFIG_FILEsubmit the recipe throughapply_srt_recipewith the shared post-eval (eval runs drop fake expert dispatch), then collect results withcopy_fixed_sequence_results/copy_agentic_resultsand eval artifacts fromlogs/eval_results. The legacy multi-node path now only serves AgentX.runners/srt-slurm/mi355x-amds.yaml: adds the multi-node settings (fabric interface, output dir, model path, HF cache mounts, exporter and nginx settings) to the single-node profile from refactor: migrate fixed-sequence recipes to SRT-Slurm / 将定长配方迁移至 SRT-Slurm #3352.CONFIG_FILE. Recipes write results in the shared sa-bench layout.sglang-disaggandvllm-disaggto synthetic acceptance.amd_utilsvLLM and ATOM engine paths, the fixed-sequence bench client, and the per-model fixed-sequence server settings.Recipes own serving and workload settings; InferenceX does not rewrite them. Workflow settings reach custom benchmarks through the exported job environment.
AgentX configs (DeepSeek V4 DSpark, GLM-5.3 TileRT) stay on the legacy path until the AgentX migration.
Folds in #2628, #2629, #2631, #2633.
Testing
Multi-node fixed-sequence e2e runs, one shape per config (8k1k). Runs at
97a721a32unless noted; the Qwen3.5 FP8 retry is at41e8bfc09.dsr1-fp4-mi355x-sglang-disaggdsr1-fp4-mi355x-sglang-disagg-mtpdsr1-fp4-mi355x-sglang-disagg-8k1k-mtpdsr1-fp4-mi355x-sglang-disagg-8k1k-mtpdsr1-fp8-mi355x-sglang-disaggdsr1-fp8-mi355x-sglang-disaggdsr1-fp8-mi355x-sglang-disagg-mtpdsr1-fp8-mi355x-sglang-disagg-mtpqwen3.5-fp4-mi355x-sglang-disaggqwen3.5-fp8-mi355x-sglang-disagg97a721a32: prefill crashed, Hub snapshot config lacksvision_config; fixed in41e8bfc09dsr1-fp4-mi355x-sglang-disaggeval-only